Separate auto-factory compiler and annotations#1814
Separate auto-factory compiler and annotations#1814CoatedMoose wants to merge 1 commit intogoogle:mainfrom
Conversation
No code changes, just moving files, adjusting POM files, and updating the README. See issue google#268.
I do not think build configuration updates are required, just recommended. The name of the "processor" package uses the same artifact identifier as the (pre-PR) "mono-package", and the new "annotations" package is a dependency of the "processor" package. None of the (java) package names have changed, so updating to the latest should be invisible. If we look at the configuration recommended in the README "Getting Started" section for the
Which is the same as the current recommended install method for this (factory) sub-project.
|
|
|
||
| Alternatively, you can include the processor itself (which transitively depends | ||
| on the annotation) in your compile-time classpath. (However, note that doing so | ||
| may pull unnecessary classes into your runtime classpath.) |
There was a problem hiding this comment.
I see that this matches https://github.com/google/auto/blob/main/value/userguide/index.md#with-maven, so that's fine for this PR. I'm just noting for the record that we should probably revisit both in the future.
My reasoning: This approach will also not work if you already have something else in annotationProcessorPaths, and it won't work in upcoming JDKs unless you pass more flags (It's possible that Maven specifically will pass those options if it doesn't already, but I also note that Gradle has been pushing users away from the classpath approach, too.)
|
|
||
| <parent> | ||
| <groupId>org.sonatype.oss</groupId> | ||
| <artifactId>oss-parent</artifactId> |
There was a problem hiding this comment.
I don't think that factory uses this yet except in its integration tests. Are we able to avoid doing so here, even if it requires pulling more plugin configuration into the aggregator or annotations build (like what you already have in processor—and see my comment about whether processor should have aggregator as its parent)?
I say this because we've had trouble with oss-parent in the past. (It's deprecated, too, probably to reflect that it's not being kept up to date with plugin versions.)
| <configuration> | ||
| <!-- disable processing because the definition in META-INF/services breaks javac --> | ||
| <compilerArgument>-proc:none</compilerArgument> | ||
| <source>1.8</source> |
There was a problem hiding this comment.
Do we need source and target here, or do we inherit them from the aggregator parent pom? If we need them, maybe we can still use java.version?
| <module>processor</module> | ||
| </modules> | ||
|
|
||
| <dependencyManagement> |
There was a problem hiding this comment.
It looks like probably few if any of these are used by annotations—which looks like it might be the only pom.xml that uses auto-factory-aggregator as its parent.
- Should the processor also use
auto-factory-aggregatoras its parent? - If not, do we need any of these dependencies here? (And could the plugin configuration be pushed down into the
annotationsbuild?) - If so, might it still make sense to push the dependencies down into
processorif that's the only place that they're used?
Basically, I can understand emptying the aggregator our (as was done in https://github.com/google/auto/pull/889/files, which I see we've also neglected...) and putting everything that each subproject needs in that subproject, and I can understand putting the intersection of what the two subprojects need into the aggregator, and I can probably understand putting the union of what the two subprojects need into the aggregator. Currently, though, we use the aggregator to configure only for annotations, and we probably configure more than we need.
3 participants
No code changes, just moving files, adjusting POM files, and updating the README.
See issue #268.
I opened a previous version of this PR in #352, there is some discussion there as well.